Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added comparison function as histogram operation (with colorized + info based string return) #763

Draft
wants to merge 28 commits into
base: develop
Choose a base branch
from

Conversation

gohil-jay
Copy link
Collaborator

image

@github-actions github-actions bot added the needs changelog Might need a changelog entry label Jul 1, 2022
if (list(self.axes)==list(hist2.axes)):
return ""
else:
return "The axes [" + "\033[91m" + str(list(self.axes)) + " and " + str(list(hist2.axes)) + "\033[0m" + "] are not equal."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return "The axes [" + "\033[91m" + str(list(self.axes)) + " and " + str(list(hist2.axes)) + "\033[0m" + "] are not equal."
return f"The axes [\033[91m{list(self.axes)} and {list(hist2.axes)}\033[0m] are not equal."

F-strings were made for this. :) Also I'd pull out the color formatting into a variable, like red and reset. Actually, there are lots of concerns with color formatting (needs to be disablable and such), at least rich can auto-format this, so probably leave the color formatting off for the moment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bet if you do import rich; rich.print(h.compare(h2)) this would still be colorful. ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I was going to do something similar (with termcolor), but then though I'd do it without importing a library..

@gohil-jay
Copy link
Collaborator Author

@henryiii, pre-commit throws few errors... Any feedback/suggestion?

@henryiii
Copy link
Member

henryiii commented Jul 3, 2022

It's complaining because you are using the result of re.search without checking to see if it has a result first. I think the best fix, though, is to use ._storage_type, which contains the storage type of the histogram, rather than trying to parse it out of the repr.

@gohil-jay
Copy link
Collaborator Author

I've been looking for something like _storage_type since the beginning (couldn't find it on docs or book here)! But alright now it should work :)

Redefined storage check & removed variance check (as variances is based on values that are already checked)
@gohil-jay
Copy link
Collaborator Author

Seems alright now (?)

@henryiii henryiii marked this pull request as draft July 5, 2022 13:11
@henryiii
Copy link
Member

Note: the goal is to support pytest_assertrepr_compare https://docs.pytest.org/en/latest/how-to/assert.html#defining-your-own-explanation-for-failed-assertions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs changelog Might need a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants